Add max_connections to OfferRequest creation #213
Conversation
| @staticmethod | ||
| def _validate_max_connections(max_connections): | ||
| """Validate the max connection number""" | ||
| if max_connections < 0: |
There was a problem hiding this comment.
we only support integers in the API, I wonder if we should:
| if max_connections < 0: | |
| if isinstance(max_connections, int) and max_connections < 0: |
There was a problem hiding this comment.
Good shout! Assuming isinstance returns True in this case when it is an int, I think we would want:
if not isinstance(max_connections, int) or max_connections < 0:
as we raise and error when True. I'll update now
There was a problem hiding this comment.
👋 Should these kinds of validations occur in the client, e.g. should these validations occur here and in the API?
There was a problem hiding this comment.
I'd agree with @sgerrand that we've generally avoided any sort of explicit type checking in the client and only performed basic consistency checks (i.e. that there are non-zero elements or certain attributes exist).
There was a problem hiding this comment.
LGTM, I can see the value in having the int check to guide developers to using the correct numeric type.
We do perform this validation on the server side as well (passing in a float results in an error), so this is valid, but we should aim to roll this out consistently in the future.
This PR adds the max_connections parameter to the OfferRequest create request.